Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to invert paint colors to be used for smart invert accessibility on iOS #6176

Merged
merged 4 commits into from
Sep 7, 2018

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Sep 5, 2018

Adds an invertColors field to the paint object. If true, a new color filter which inverts the paint will be added (combined with any pending color filters).

Since I believe this will overwrite any user provided color filters, an alternative would be to (from the framework side) instead push a layer specifically to invert the image. This would require either creating a new type of ColorFilter or a special layer to do this. Perhaps it is also possible to merge two color filters in some way?

Updated with suggestion below, we can use both color filters!

Note: I cannot take a screenshot of this working properly because smart invert seems to get toggled off for them.

cc @liyuqian

@@ -30,6 +30,7 @@ constexpr int kColorFilterBlendModeIndex = 11;
constexpr int kMaskFilterIndex = 12;
constexpr int kMaskFilterBlurStyleIndex = 13;
constexpr int kMaskFilterSigmaIndex = 14;
constexpr int kInvertColorIndex = 15;
constexpr size_t kDataByteCount = 75; // 4 * (last index + 1)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Hixie - this calculation seems to be a bit off

@brianosman
Copy link
Contributor

brianosman commented Sep 6, 2018

I'm not sure of the full context here, but you can compose two color filters together. See SkColorFilter::makeComposed().

sk_sp<SkColorFilter> original = ...;
sk_sp<SkColorFilter> invert = ...;
// Make a new filter that will apply 'original', then 'invert'
sk_sp<SkColorFilter> composed = invert->makeComposed(original);

@jonahwilliams
Copy link
Member Author

@brianosman thats perfect!

For context, iOS has an accessibility feature called "smart invert" which inverts screen colors, except for images. Naturally since this is implemented in UIView, flutter apps are entirely inverted. My solution to fix this is to detect when smart invert is enabled and then selectively invert the paint used for images in a flutter app, so that when iOS inverts the screen colors the images end up looking normal.

@@ -47,6 +48,11 @@ constexpr uint32_t kBlendModeDefault =
// default SkPaintDefaults_MiterLimit in Skia (which is not in a public header).
constexpr double kStrokeMiterLimitDefault = 4.0;

// A color matrix which inverts colors.
constexpr SkScalar invert_colors[20] = {-1.0, 0, 0, 1.0, 0, 0, -1.0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format this in a nice 4x5 matrix? The current 3x* matrix is hard to read...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what clang-format gave me :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be helpful

@@ -116,7 +122,19 @@ Paint::Paint(Dart_Handle paint_objects, Dart_Handle paint_data) {
if (filter_quality)
paint_.setFilterQuality(static_cast<SkFilterQuality>(filter_quality));

if (uint_data[kColorFilterIndex]) {
if (uint_data[kColorFilterIndex] && uint_data[kInvertColorIndex]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we have unit tests or golden tests in flutter/flutter that exercise the two new branches that you added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't but I can add some, like solid paint with invert applied, solid paint with invert and color filter applied. That should cover this branch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the existing set of golden tests should cover everything, and then when I map the invert color flag to the accessibility setting after rolling the engine I can add special test cases? I'm not sure of another way I could write the tests for this

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (assuming that we'll have corresponding tests soon 😄)

@jonahwilliams jonahwilliams merged commit 687cf08 into flutter:master Sep 7, 2018
@jonahwilliams jonahwilliams deleted the invert_colors branch September 7, 2018 17:22
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2018
flutter/engine@58a1894...687cf08

git log 58a1894..687cf08 --no-merges --oneline
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2018
flutter/engine@58a1894...2af72eb

git log 58a1894..2af72eb --no-merges --oneline
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2018
flutter/engine@58a1894...16c56af

git log 58a1894..16c56af --no-merges --oneline
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2018
flutter/engine@58a1894...93dac2a

git log 58a1894..93dac2a --no-merges --oneline
93dac2a Always save canvas and correctly pass antialias boolean in ClipRects. (flutter/engine#6199)
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2018
flutter/engine@58a1894...e27a2e9

git log 58a1894..e27a2e9 --no-merges --oneline
e27a2e9 Dart SDK roll for 2018/09/07 (flutter/engine#6201)
93dac2a Always save canvas and correctly pass antialias boolean in ClipRects. (flutter/engine#6199)
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2018
flutter/engine@58a1894...bf96dbe

git log 58a1894..bf96dbe --no-merges --oneline
bf96dbe Revert &#34;Some cleanups enabled by removing support for Dart. (flutter#5621)&#34; (flutter/engine#6205)
3358115 Roll src/third_party/skia 094b3eaf479c..2810c856dfa2 (6 commits) (flutter/engine#6203)
4c2448d Some cleanups enabled by removing support for Dart. (flutter/engine#5621)
e27a2e9 Dart SDK roll for 2018/09/07 (flutter/engine#6201)
93dac2a Always save canvas and correctly pass antialias boolean in ClipRects. (flutter/engine#6199)
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2018
flutter/engine@58a1894...b952331

git log 58a1894..b952331 --no-merges --oneline
b952331 Allow embedders to specify a custom GL proc address resolver. (flutter/engine#6204)
bf96dbe Revert &#34;Some cleanups enabled by removing support for Dart. (flutter#5621)&#34; (flutter/engine#6205)
3358115 Roll src/third_party/skia 094b3eaf479c..2810c856dfa2 (6 commits) (flutter/engine#6203)
4c2448d Some cleanups enabled by removing support for Dart. (flutter/engine#5621)
e27a2e9 Dart SDK roll for 2018/09/07 (flutter/engine#6201)
93dac2a Always save canvas and correctly pass antialias boolean in ClipRects. (flutter/engine#6199)
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2018
flutter/engine@58a1894...bf3c224

git log 58a1894..bf3c224 --no-merges --oneline
bf3c224 Roll src/third_party/skia 2810c856dfa2..a3dc329d1db1 (1 commits) (flutter/engine#6206)
b952331 Allow embedders to specify a custom GL proc address resolver. (flutter/engine#6204)
bf96dbe Revert &#34;Some cleanups enabled by removing support for Dart. (flutter#5621)&#34; (flutter/engine#6205)
3358115 Roll src/third_party/skia 094b3eaf479c..2810c856dfa2 (6 commits) (flutter/engine#6203)
4c2448d Some cleanups enabled by removing support for Dart. (flutter/engine#5621)
e27a2e9 Dart SDK roll for 2018/09/07 (flutter/engine#6201)
93dac2a Always save canvas and correctly pass antialias boolean in ClipRects. (flutter/engine#6199)
16c56af Roll src/third_party/skia 300b6197e344..094b3eaf479c (17 commits) (flutter/engine#6200)
2af72eb Don&#39;t map iOS reduce motion to disabled animations (flutter/engine#6194)
687cf08 Add option to invert paint colors to be used for smart invert accessibility on iOS (flutter/engine#6176)
85d47fb Accept file URIs as parameters in the RunInView service RPC (flutter/engine#6191)
5a19d76 Roll src/third_party/skia 0b80e62a14ae..300b6197e344 (9 commits) (flutter/engine#6198)
f33615c Roll src/third_party/skia b090b2b26803..0b80e62a14ae (2 commits) (flutter/engine#6197)
75f6bdf Revert &#34;Dart SDK roll for 2018/09/06 (flutter#6189)&#34; (flutter/engine#6192)
b09563b Roll src/third_party/skia 106d04e907c1..b090b2b26803 (1 commits) (flutter/engine#6193)
e164e83 Reset the raster cache when the compositor context is created. (flutter/engine#6150)
f02fc8c Dart SDK roll for 2018/09/06 (flutter/engine#6189)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
amirh pushed a commit to amirh/engine that referenced this pull request Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants